Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix assertion for test_katello_certs_check_output_invalid_input #17002

Merged
merged 1 commit into from
Dec 3, 2024
Merged

Fix assertion for test_katello_certs_check_output_invalid_input #17002

merged 1 commit into from
Dec 3, 2024

Conversation

jameerpathan111
Copy link
Contributor

Problem Statement

  • test_katello_certs_check_output_invalid_input[error0-certs/invalid.crt-certs/invalid.key-certs/ca.crt] is failing with AssertionError because the error message has changed.

Solution

  • Fix assertion

Related Issues

  • SAT-29725

@jameerpathan111 jameerpathan111 added CherryPick PR needs CherryPick to previous branches AutoMerge_Cherry_Picked The cherrypicked PRs of master PR would be automerged if all checks passing Stream Introduced in or relating directly to Satellite Stream/Master 6.16.z Introduced in or relating directly to Satellite 6.16 labels Nov 26, 2024
@jameerpathan111 jameerpathan111 self-assigned this Nov 26, 2024
@jameerpathan111 jameerpathan111 requested a review from a team as a code owner November 26, 2024 14:47
@jameerpathan111
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/sys/test_katello_certs_check.py

@evgeni
Copy link
Member

evgeni commented Nov 26, 2024

This looks like a raw OpenSSL error. Did that change in both EL8 and EL9?

@jameerpathan111
Copy link
Contributor Author

This looks like a raw OpenSSL error. Did that change in both EL8 and EL9?

Hmm, it's failing only for EL9 on 6.16

@evgeni
Copy link
Member

evgeni commented Nov 26, 2024

This looks like a raw OpenSSL error. Did that change in both EL8 and EL9?

Hmm, it's failing only for EL9 on 6.16

Then ACK on master/Stream, but you're going to need a different patch for 6.16 :/

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 9464
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/sys/test_katello_certs_check.py --external-logging
Test Result : ========== 5 passed, 1 deselected, 38 warnings in 1475.36s (0:24:35) ===========

@Satellite-QE Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label Nov 26, 2024
@jameerpathan111
Copy link
Contributor Author

jameerpathan111 commented Nov 26, 2024 •
edited
Loading

This looks like a raw OpenSSL error. Did that change in both EL8 and EL9?

Hmm, it's failing only for EL9 on 6.16

Then ACK on master/Stream, but you're going to need a different patch for 6.16 :/

Can I just keep error 26 at 0 depth lookup: or error /root/certs/invalid.crt: verification failed(It's in both EL9 and EL8)?😄

@Satellite-QE Satellite-QE removed the PRT-Passed Indicates that latest PRT run is passed for the PR label Nov 26, 2024
@evgeni
Copy link
Member

evgeni commented Nov 26, 2024

If the assertion can use a substring, sure

@jameerpathan111
Copy link
Contributor Author

jameerpathan111 commented Nov 27, 2024 •
edited
Loading

If the assertion can use a substring, sure

maybe let's not, kept the original message. Updating it based on rhel version.

@jameerpathan111
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/sys/test_katello_certs_check.py

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 9470
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/sys/test_katello_certs_check.py --external-logging
Test Result : ========== 5 passed, 1 deselected, 37 warnings in 1411.15s (0:23:31) ===========

@Satellite-QE Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label Nov 27, 2024
ekohl
ekohl reviewed Nov 28, 2024
View reviewed changes
tests/foreman/sys/test_katello_certs_check.py
@@ -73,11 +73,12 @@ class TestKatelloCertsCheck:
CA cert (a.k.a cacert.crt or rootCA.pem) can be used as bundle file.
"""

error_message = f"error 26 at 0 depth lookup: {'unsupported' if settings.repos.rhel_major_version == '8' else 'unsuitable'} certificate purpose"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error surprises me a bit. If we don't assert the explicit message we emit ourselves:
https://github.com/theforeman/foreman-installer/blob/7c2b54267a933bcf0261396c0950d51c8d5bec5c/bin/katello-certs-check#L157-L169

Perhaps that's not needed and we can simply assert the right code path is used by checking for exit code 4?

Copy link
Contributor Author

@jameerpathan111 jameerpathan111 Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is what I see in logs, no exit code 4

    
    Checking CA bundle against the certificate file: 
    [FAIL]
    
    The /root/cacert.crt does not verify the /root/certs/invalid.crt
    CN=foreman.example.com
    error 20 at 0 depth lookup: unable to get local issuer certificate
    error /root/certs/invalid.crt: verification failed
    
    Checking CA bundle size: 1
    [OK]
    
    Checking if CA bundle has trust rules: 0
    [OK]
    
    Checking Subject Alt Name on certificate 
    [FAIL]
    
    The /root/certs/invalid.crt does not contain a Subject Alt Name. Common Name is deprecated, use Subject Alt Name instead. See: https://tools.ietf.org/html/rfc2818#section-3.1
    Checking Key Usage extension on certificate for Key Encipherment 
    [OK]
    
    Checking for use of shortname as CN
    
    stderr:
    
    status: 1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That signals something is broken. The error function sets the exit code:
https://github.com/theforeman/foreman-installer/blob/7c2b54267a933bcf0261396c0950d51c8d5bec5c/bin/katello-certs-check#L65-L70

Then It's also surprising the command exits unexpectedly, without showing [OK] or [FAIL]. That looks like check-shortname isn't following an expected code path:
https://github.com/theforeman/foreman-installer/blob/7c2b54267a933bcf0261396c0950d51c8d5bec5c/bin/katello-certs-check#L229-L250

Given there was already a warning that it doesn't contain a SAN I think we can conclude it's missing an else statement with success.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it's a bug😅. DO you want me to file an issue?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great to have it covered in our tests: https://github.com/theforeman/foreman-installer/blob/develop/spec/katello_certs_check_spec.rb is what we use in CI.

Do I read it well that these certs are generated using tests/foreman/data/certs.sh? In particular:

robottelo/tests/foreman/data/certs.sh

Lines 48 to 49 in fdad16a

# generate invalid certs with hostname as 'foreman.example.com'
generate_certs "invalid" "/CN=foreman.example.com" "client_extensions"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

@Satellite-QE Satellite-QE removed the PRT-Passed Indicates that latest PRT run is passed for the PR label Nov 28, 2024
@lpramuk lpramuk merged commit 2908992 into SatelliteQE:master Dec 3, 2024
8 checks passed
github-actions bot pushed a commit that referenced this pull request Dec 3, 2024
@jameerpathan111 @web-flow
Fix assertion for test_katello_certs_check_output_invalid_input (#17002)
d6fc09b 
(cherry picked from commit 2908992)
@Satellite-QE Satellite-QE mentioned this pull request Dec 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekohl ekohl ekohl approved these changes

@rmynar rmynar rmynar approved these changes

@lpramuk lpramuk lpramuk approved these changes

@ehelms ehelms ehelms approved these changes

@evgeni evgeni evgeni approved these changes

Assignees

@jameerpathan111 jameerpathan111

Labels
6.16.z Introduced in or relating directly to Satellite 6.16 AutoMerge_Cherry_Picked The cherrypicked PRs of master PR would be automerged if all checks passing CherryPick PR needs CherryPick to previous branches Stream Introduced in or relating directly to Satellite Stream/Master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jameerpathan111 @evgeni @Satellite-QE @ekohl @ehelms @lpramuk @rmynar